-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Metagenomics Exchange upload command and related bookkeeping code #350
Conversation
- Refactored the cli wrapper - Refactored the settings to be in line with the rest - Modified the unit tests
The test tests/me/test_populate_metagenomics_exchange.py::TestMeAPI::test_removals_dry_mode it's working now.
…nomics/emgapi into feature/metagenomics_exchange
…ndexed This is needed because RenameField doesn't allow you to change the db_column name. With this change it seems to work, and the data should be kept: ```sql -- -- Alter field last_indexed on analysisjob -- ALTER TABLE "ANALYSIS_JOB" RENAME COLUMN "LAST_INDEXED" TO "LAST_EBI_SEARCH_INDEXED"; -- -- Alter field last_indexed on study -- ALTER TABLE "STUDY" RENAME COLUMN "LAST_INDEXED" TO "LAST_EBI_SEARCH_INDEXED"; -- -- Rename field last_indexed on analysisjob to last_ebi_search_indexed -- -- -- Rename field last_indexed on study to last_ebi_search_indexed -- ```
…change Feature/metagenomics exchange
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mberacochea
Thanks for doing this.
Functionally speaking it all looks fine to me.
I just left a few minor comments that could perhaps improve the code in the long term.
But I don't think they are blockers for releasing this.
emgapi/metagenomics_exchange.py
Outdated
logging.warning(f"Get API request failed") | ||
return analysis_registry_id , metadata_match | ||
|
||
if response.ok: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think reducing the amount of nesting, can make this code more easier to maintain in the future.
Specifically referring to Lines: 111-141
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks folks. Nothing much to add from me, other than:
- we will need a change to the config repo to add the ME config for dev and prod.
- We now usually include an API version bump on
develop
so that the pip installing themaster
branch always installs the latest commit.
PR 👍 |
Add support for Assembly and Run based jobs (both). Remove the public check, we only submit public data to the exchange. Normalize argument names in the MGX wrapper. Added some docstrings
The MGX API returns a json with the error message, which is useful for debugging.
…hange-bug-fixes Fixes for a few bugs I encountered while testing the command
No description provided.